🦋 Changeset detected

Latest commit: 95715f9

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Storybook has this code in the vite-builder:

https://github.com/storybookjs/storybook/blob/86ca49742641b6cc36a300ddff768da618ccb67c/code/builders/builder-vite/src/codegen-modern-iframe-script.ts#L32

  const generateHMRHandler = (frameworkName: string): string => {
    // Web components are not compatible with HMR, so disable HMR, reload page instead.
    if (frameworkName === '@storybook/web-components-vite') {
      return `
      if (import.meta.hot) {
        import.meta.hot.decline();
      }`.trim();
    }

Do you think we need it too? Or is your solution not using import.meta.hot?

Storybook has this code in the vite-builder:

https://github.com/storybookjs/storybook/blob/86ca49742641b6cc36a300ddff768da618ccb67c/code/builders/builder-vite/src/codegen-modern-iframe-script.ts#L32

  const generateHMRHandler = (frameworkName: string): string => {
    // Web components are not compatible with HMR, so disable HMR, reload page instead.
    if (frameworkName === '@storybook/web-components-vite') {
      return `
      if (import.meta.hot) {
        import.meta.hot.decline();
      }`.trim();
    }

Do you think we need it too? Or is your solution not using import.meta.hot?

I think I figured myself. We don't have import.meta.hot in the preview iframe unless we configure dev-server-hmr plugin, which we don't do right now. I know we don't need real HMR, but maybe we could reuse it's API to reload page instead? Curious if you considered this idea? In any case, I like your current solution too, and it seems to be the same way as it was with storybook-prebuilt and it's good from the Storybook Builder API perspective too.

I think I figured myself. We don't have import.meta.hot in the preview iframe unless we configure dev-server-hmr plugin, which we don't do right now. I know we don't need real HMR, but maybe we could reuse it's API to reload page instead? Curious if you considered this idea?

I had not run into this as a path. It seems like HMR is mainly handled in Vite as opposed to Storybook, which might mean we'd need expanded support for it in WDS to take this path? I could spend some more time investigating this path if it felt productive, might take a little time for me to get to the bottom of this side.

I think I figured myself. We don't have import.meta.hot in the preview iframe unless we configure dev-server-hmr plugin, which we don't do right now. I know we don't need real HMR, but maybe we could reuse it's API to reload page instead? Curious if you considered this idea?

I had not run into this as a path. It seems like HMR is mainly handled in Vite as opposed to Storybook, which might mean we'd need expanded support for it in WDS to take this path? I could spend some more time investigating this path if it felt productive, might take a little time for me to get to the bottom of this side.

If we start supporting a wider range of frameworks in the future, e.g. Angular as it seems to be recommending WTR right, will we be able to add HMR without breaking this setup?

It seems like HMR is mainly handled in Vite as opposed to Storybook

Correct, it's a builder's feature in other words. Even if we include the plugin dev-server-hmr, not sure it's gonna work in a middleware mode without your change though, because somewhere the WebSockets needs to be setup

There are some docs about the larger HMR plugin ecosystem: https://modern-web.dev/docs/dev-server/plugins/hmr/

It also requires web socket access:

https://github.com/modernweb-dev/web/blob/master/packages/dev-server-hmr/src/HmrPlugin.ts#L70-L72

This PR seems ready to me :)
Can I assume you were testing it in your storybook when developing?

I need to update my local patches for the changes from our conversation, but yes. I should be able to test that to be sure today and then I’ll report back!

This is now working for me locally in a test project and in SWC. If you're interested in testing it out, https://github.com/westbrook/wds-storybook-test and https://github.com/adobe/spectrum-web-components/tree/storybook both have equivalent patches to this change.

Storybook released https://www.npmjs.com/package/@storybook/addon-a11y/v/7.5.0 yesterday, which was also blocking my adoption, so it's all getting quite exciting now!! How are we looking on this PR?

Awesome news! Thanks for your work @Westbrook !